feat: support experimental.serverActions.bodySizeLimit from next.config#338
Conversation
|
/bigbonk review |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good feature addition — the 1MB hardcoded limit was a real pain point for file uploads. The overall architecture is correct: config parsing → resolved config → plumbed through generateRscEntry → embedded in generated code. The production server and Cloudflare worker entry inherit the limit from the built RSC entry, so they're covered automatically.
Tests pass and cover the main cases well. A few issues to address below, ranging from a behavioral mismatch with Next.js to minor suggestions.
| * Accepts Next.js-style strings like "1mb", "500kb", "10mb". | ||
| * Returns the default 1MB if the value is not provided or invalid. | ||
| */ | ||
| export function parseBodySizeLimit(value: string | number | undefined | null): number { |
There was a problem hiding this comment.
Minor: Next.js validates that the parsed value is >= 1 and throws an error for negative numbers or zero (see packages/next/src/server/config.ts). This implementation silently accepts 0, negative numbers, and "0mb" as valid. Consider adding validation:
if (value <= 0) return 1 * 1024 * 1024;or even throwing, to match Next.js behavior:
if (value === null || isNaN(value) || value < 1) {
throw new Error(
'Server Actions Size Limit must be a valid number or filesize format larger than 1MB: https://nextjs.org/docs/app/api-reference/next-config-js/serverActions#bodysizelimit'
);
}Not a blocker, but something to consider for parity.
| * Prevents unbounded request body buffering. | ||
| */ | ||
| var __MAX_ACTION_BODY_SIZE = 1 * 1024 * 1024; | ||
| var __MAX_ACTION_BODY_SIZE = ${JSON.stringify(bodySizeLimit)}; |
There was a problem hiding this comment.
JSON.stringify(bodySizeLimit) works correctly here since bodySizeLimit is always a number — it'll produce e.g. 10485760 in the generated code. Just noting for future readers that JSON.stringify on a number is equivalent to String() / template interpolation, so this is fine.
|
Review submitted. Here's a summary of my feedback on PR #338: Overall: The PR correctly implements the feature. The config parsing pipeline ( Key feedback:
|
- Accept bare number strings (e.g. "1048576") as bytes, matching Next.js bytes library behavior - Add tb and pb unit support - Throw on zero/negative values to match Next.js validation behavior - Remove unreachable type guard (value is always string at that point) - Update tests: add tb/pb/bare-number/validation cases, fix "10tb" assertion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
youanden
force-pushed
the
feat/body-size-limit
branch
from
7a9b6d7 to
226f033
Compare
|
/bigbonk review this pr please |
There was a problem hiding this comment.
Good work. This is a well-scoped fix for a real problem — the hardcoded 1MB limit was silently breaking file uploads. The plumbing is correct: next.config → resolveNextConfig → index.ts → generateRscEntry → embedded __MAX_ACTION_BODY_SIZE. Production builds inherit the value from the built RSC entry, so no changes needed there. Tests are thorough and all pass.
I have two minor notes below, neither blocking.
| } | ||
| const trimmed = value.trim(); | ||
| const match = trimmed.match(/^(\d+(?:\.\d+)?)\s*(b|kb|mb|gb|tb|pb)?$/i); | ||
| if (!match) return 1 * 1024 * 1024; |
There was a problem hiding this comment.
Inconsistent error handling between invalid strings and invalid numbers: parseBodySizeLimit(0) throws, but parseBodySizeLimit("abc") silently returns the 1MB default. A user who typos their config as bodySizeLimit: "10mbb" would get no feedback that their setting was ignored.
Consider logging a warning when the regex doesn't match a non-empty string, so misconfiguration doesn't go unnoticed:
| if (!match) return 1 * 1024 * 1024; | |
| if (!match) { | |
| console.warn(`[vinext] Could not parse serverActions.bodySizeLimit value "${value}", using default 1MB`); | |
| return 1 * 1024 * 1024; | |
| } |
Not blocking — the silent default matches how vinext handles most config fallbacks, so this is a style call.
| case "pb": bytes = Math.floor(num * 1024 * 1024 * 1024 * 1024 * 1024); break; | ||
| default: return 1 * 1024 * 1024; | ||
| } | ||
| if (bytes < 1) throw new Error(`Body size limit must be a positive number, got ${bytes}`); |
There was a problem hiding this comment.
This bytes < 1 check after the switch can only trigger for string inputs like "0b" or "0.5b" (which floors to 0). For numeric inputs, the value < 1 check on line 24 already covers this.
The string "0mb" also produces bytes = 0 and throws here, which is good. But "0.001kb" produces Math.floor(0.001 * 1024) = 1 — which passes. This is fine in practice, just noting the edge for awareness.
|
Review submitted with approval. Here's a summary: Verdict: Approved The PR correctly wires Two non-blocking notes I left:
|
… back Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/bigbonk re-review please! |
There was a problem hiding this comment.
Clean, well-scoped feature. The plumbing from next.config → resolveNextConfig → index.ts → generateRscEntry → embedded __MAX_ACTION_BODY_SIZE is correct. Production builds inherit the value from the built RSC entry, so no changes needed there. The Pages Router bodyParser.sizeLimit in index.ts:1570 and api-handler.ts:40 are separate concerns and correctly left untouched.
The author has addressed the feedback from previous reviews: bare number strings work, tb/pb are supported, invalid strings warn instead of silently falling back, and zero/negative values throw. Tests are thorough (27 passing). Two small non-blocking notes below.
| case "pb": bytes = Math.floor(num * 1024 * 1024 * 1024 * 1024 * 1024); break; | ||
| default: return 1 * 1024 * 1024; | ||
| } | ||
| if (bytes < 1) throw new Error(`Body size limit must be a positive number, got ${bytes}`); |
There was a problem hiding this comment.
Nit: the bytes < 1 check catches "0b", "0mb", and "0.5b" (floors to 0), which is good — but none of these string-zero cases are tested. The test only covers numeric 0 and -1. Consider adding a quick assertion for string edge cases:
expect(() => parseBodySizeLimit("0b")).toThrow();
expect(() => parseBodySizeLimit("0mb")).toThrow();Not blocking.
| export function parseBodySizeLimit(value: string | number | undefined | null): number { | ||
| if (value === undefined || value === null) return 1 * 1024 * 1024; | ||
| if (typeof value === "number") { | ||
| if (value < 1) throw new Error(`Body size limit must be a positive number, got ${value}`); |
There was a problem hiding this comment.
Minor inconsistency: numeric 0.5 passes (it's >= 1 as false wait — 0.5 < 1 is true, so this throws, which is correct). But a numeric 1.5 is accepted as-is (1.5 bytes), while a string "1.5b" gets Math.floor'd to 1. This is consistent with how Next.js's bytes library works (numeric passthrough, string parsing floors), so it's fine — just noting it for awareness.
|
Review submitted with approval. Here's a summary: Verdict: Approved The PR correctly wires The implementation addresses all previous review feedback — bare number strings, Two non-blocking notes I left:
|
2 participants
Adds support for reading experimental.serverActions.bodySizeLimit from next.config.ts and using it to configure the server action request body size limit. Currently, vinext hardcodes
__MAX_ACTION_BODY_SIZEto 1MB (1 * 1024 * 1024), ignoring any user configuration and causing 413 Payload Too Large errors when uploading files >1MB via server actions.Next.js respects this setting (docs), but vinext's generated dev server entry hardcodes the limit.
Problem
I faced an issue uploading files above 1MB in dev mode that drove this PR's creation. I had created a patch in my repo beforehand to correct the problem locally:
Test plan
Added 14 unit tests covering string parsing (mb/kb/gb/b), numeric passthrough, case insensitivity, fractional values, and default/invalid fallbacks
Added integration tests verifying
resolveNextConfig()end-to-end withexperimental.serverActions.bodySizeLimitAll existing tests continue to pass, oxlint was clean, typecheck had 4 existing errors prior to my changes.